introduce dirty list to dataflow #51900

PramodBisht · 2018-06-29T13:40:03Z

@nikomatsakis my naive implementation never worked, So, I decided to implement using work_queue data structure. This PR also includes your commits from nll-liveness-dirty-list branch. Those commits should not visible once your branch is merged.

r? @nikomatsakis

nikomatsakis · 2018-06-29T13:50:13Z

src/librustc_mir/dataflow/mod.rs

-            builder.propagate_bits_into_graph_successors_of(
-                in_out, &mut self.changed, (mir::BasicBlock::new(bb_idx), bb_data));
+        while let Some(bb) = dirty_queue.pop() {
+            let bb_data: &BasicBlockData<'tcx> = &mir.basic_blocks()[*bb];


easier: &mir[bb]

also, no type annotation is needed here (but it doens't hurt)

nikomatsakis · 2018-06-29T13:50:43Z

src/librustc_mir/dataflow/mod.rs

-                in_out, &mut self.changed, (mir::BasicBlock::new(bb_idx), bb_data));
+        while let Some(bb) = dirty_queue.pop() {
+            let bb_data: &BasicBlockData<'tcx> = &mir.basic_blocks()[*bb];
+                let builder = &mut self.builder;


odd indentation; and why introduce let?

fixed indentation, let is introduced in diff because of bad indentation. Anyway, I have removed the dependency of that builder variable.

nikomatsakis · 2018-06-29T13:52:12Z

src/librustc_mir/dataflow/mod.rs

+                {
+                    let sets = builder.flow_state.sets.for_block(bb_idx);
+                    debug_assert!(in_out.words().len() == sets.on_entry.words().len());
+                    in_out.clone_from(sets.on_entry);


@nnethercote noticed that this name (clone_from) was causing problems (it shadows clone_from from the Clone trait, and sometimes that gets called instead, which is less efficient). They have a PR to fix it, but in the meantime, do this, which will ensure we're calling the method we want:

IdxSet::clone_from(in_out, sets.on_entry);

that said, I think we will be calling the correct one here anyway, given the type of in_out.

nikomatsakis · 2018-06-29T13:52:58Z

src/librustc_mir/dataflow/mod.rs

+                    in_out.subtract(sets.kill_set);
+                }
+                builder.propagate_bits_into_graph_successors_of(
+                    in_out, &mut self.changed, (mir::BasicBlock::new(bb_idx), bb_data), dirty_queue);


I don't think we need the changed flag anymore -- the only bit that matters is if the dirty queue is empty.

removed changed flag.

nikomatsakis · 2018-06-29T13:53:38Z

src/librustc_mir/dataflow/mod.rs

        let entry_set = self.flow_state.sets.for_block(bb.index()).on_entry;
        let set_changed = bitwise(entry_set.words_mut(),
                                  in_out.words(),
                                  &self.flow_state.operator);
        if set_changed {
            *changed = true;
+            for row in self.mir.basic_blocks()[*bb].terminator().successors(){


Actually, bb here is already the successor -- we just need to do

dirty_list.insert(*bb);

nikomatsakis · 2018-06-29T13:53:53Z

src/librustc_mir/dataflow/mod.rs

@@ -877,13 +885,18 @@ impl<'a, 'tcx: 'a, D> DataflowAnalysis<'a, 'tcx, D> where D: BitDenotation
    fn propagate_bits_into_entry_set_for(&mut self,
                                         in_out: &IdxSet<D::Idx>,
                                         changed: &mut bool,
-                                         bb: &mir::BasicBlock) {
+                                         bb: &mir::BasicBlock,


pre-existing, but I think we should change the type of bb here to bb: mir::BasicBlock -- the & is silly

nikomatsakis

This looks functionally correct! Exciting. I left some nits.

nikomatsakis · 2018-06-29T17:05:33Z

src/librustc_mir/dataflow/mod.rs

-            propcx.changed = false;
-            propcx.walk_cfg(&mut temp);
-        }
+        let mut dirty_queue: WorkQueue<mir::BasicBlock> =


Nit: we should probably move the dirty_queue creation into walk_cfg; or else merge walk_cfg and this method. But anyway, doesn't matter.

nikomatsakis · 2018-06-29T17:06:18Z

src/librustc_mir/dataflow/mod.rs

-            builder.propagate_bits_into_graph_successors_of(
-                in_out, &mut self.changed, (mir::BasicBlock::new(bb_idx), bb_data));
+            self.builder.propagate_bits_into_graph_successors_of(
+                in_out, (mir::BasicBlock::new(bb.index()), bb_data), dirty_queue);


Nit: mir::BasicBlock::new(bb.index()) is just bb =)

nikomatsakis · 2018-06-29T17:07:01Z

src/librustc_mir/dataflow/mod.rs

            mir::TerminatorKind::Goto { ref target } |
            mir::TerminatorKind::Assert { ref target, cleanup: None, .. } |
            mir::TerminatorKind::Yield { resume: ref target, drop: None, .. } |
            mir::TerminatorKind::Drop { ref target, location: _, unwind: None } |
            mir::TerminatorKind::DropAndReplace {
                ref target, value: _, location: _, unwind: None
            } => {
-                self.propagate_bits_into_entry_set_for(in_out, changed, target);
+                self.propagate_bits_into_entry_set_for(in_out, *target, dirty_list);


Nit: instead of *target here, we can remove the ref from ref target in the patterns above

nikomatsakis · 2018-06-29T17:07:12Z

src/librustc_mir/dataflow/mod.rs

    {
        match bb_data.terminator().kind {
            mir::TerminatorKind::Return |
            mir::TerminatorKind::Resume |
            mir::TerminatorKind::Abort |
            mir::TerminatorKind::GeneratorDrop |
            mir::TerminatorKind::Unreachable => {}
+
            mir::TerminatorKind::Goto { ref target } |


Nit: i.e., change this to target not ref target (see below)

nikomatsakis · 2018-06-29T17:07:27Z

src/librustc_mir/dataflow/mod.rs

            }
            mir::TerminatorKind::Assert { ref target, cleanup: Some(ref unwind), .. } |
            mir::TerminatorKind::Drop { ref target, location: _, unwind: Some(ref unwind) } |
            mir::TerminatorKind::DropAndReplace {
                ref target, value: _, location: _, unwind: Some(ref unwind)
            } => {
-                self.propagate_bits_into_entry_set_for(in_out, changed, target);
+                self.propagate_bits_into_entry_set_for(in_out, *target, dirty_list);


Nit: same here...

(and all the other cases below)

nikomatsakis · 2018-06-30T09:14:39Z

@bors try

Local perf runs suggested this was not a win.

bors · 2018-06-30T09:14:50Z

⌛ Trying commit 2a5c3c469680e333ec06845b725cd07aaf1fae45 with merge ddc3f99bad126b9f5755177d82f651d8f91d8683...

bors · 2018-06-30T10:52:43Z

☀️ Test successful - status-travis
State: approved= try=True

lqd · 2018-06-30T16:55:49Z

@Mark-Simulacrum can we schedule a perf run for this PR please ? :)

Mark-Simulacrum · 2018-06-30T17:01:42Z

Perf run is scheduled.

nikomatsakis · 2018-07-02T16:01:40Z

OK, the perf results are quite interesting:

http://perf.rust-lang.org/compare.html?start=87ecf5442ced38a6253e670dd6d87c0c334b21fb&end=ddc3f99bad126b9f5755177d82f651d8f91d8683&stat=instructions%3Au

It seems like my local measurement was correct but misleading -- clap-rs did get slower (by 2.5%) but lots of stuff is way faster!

nikomatsakis · 2018-07-02T17:17:16Z

This is r=me but:

it needs rebase
probably oughta squash / cleanup the history
it is blocked waiting on introduce dirty list to liveness, eliminate ins vector #51896

nikomatsakis · 2018-07-02T18:45:08Z

src/librustc_mir/dataflow/mod.rs

                debug_assert!(in_out.words().len() == sets.on_entry.words().len());
-                in_out.overwrite(sets.on_entry);
+                IdxSet::clone_from(in_out, sets.on_entry);


should be overwrite

nikomatsakis · 2018-07-02T21:04:28Z

@bors r+

bors · 2018-07-02T21:04:29Z

📌 Commit 09df6a0 has been approved by nikomatsakis

bors · 2018-07-03T21:48:44Z

⌛ Testing commit 09df6a0 with merge fb97bb5...

@nikomatsakis

introduce dirty list to dataflow @nikomatsakis my naive implementation never worked, So, I decided to implement using `work_queue` data structure. This PR also includes your commits from `nll-liveness-dirty-list` branch. Those commits should not visible once your branch is merged. r? @nikomatsakis

bors · 2018-07-03T23:48:26Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing fb97bb5 to master...

michaelwoerister · 2018-07-05T08:32:52Z

These are some pretty great performance wins for some crates even in the non-NLL case! 👍

rust-highfive assigned nikomatsakis Jun 29, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 29, 2018

PramodBisht requested a review from nikomatsakis June 29, 2018 13:40

This comment has been minimized.

Sign in to view

nikomatsakis requested changes Jun 29, 2018

View reviewed changes

This comment has been minimized.

Sign in to view

PramodBisht force-pushed the 51813_b branch from ad63d4f to e37e50b Compare June 29, 2018 15:42

nikomatsakis reviewed Jun 29, 2018

View reviewed changes

nikomatsakis mentioned this pull request Jun 29, 2018

[nll] introduce dirty list to dataflow #51813

Closed

nikomatsakis approved these changes Jun 30, 2018

View reviewed changes

nikomatsakis changed the title ~~[WIP] introduce dirty list to dataflow~~ introduce dirty list to dataflow Jul 2, 2018

nikomatsakis added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 2, 2018

PramodBisht force-pushed the 51813_b branch from 2a5c3c4 to 9cdd31a Compare July 2, 2018 18:11

This comment has been minimized.

Sign in to view

nikomatsakis requested changes Jul 2, 2018

View reviewed changes

PramodBisht force-pushed the 51813_b branch from 9cdd31a to 827a0d1 Compare July 2, 2018 18:49

Address rust-lang#51813

09df6a0

PramodBisht force-pushed the 51813_b branch from 827a0d1 to 09df6a0 Compare July 2, 2018 20:30

nikomatsakis approved these changes Jul 2, 2018

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jul 2, 2018

bors merged commit 09df6a0 into rust-lang:master Jul 3, 2018

pnkfelix mentioned this pull request May 1, 2020

Use existing framework for backward dataflow analyses #71006

Merged

introduce dirty list to dataflow #51900

introduce dirty list to dataflow #51900

Conversation

PramodBisht commented Jun 29, 2018

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis Jun 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Jun 30, 2018

bors commented Jun 30, 2018

bors commented Jun 30, 2018

lqd commented Jun 30, 2018

Mark-Simulacrum commented Jun 30, 2018

nikomatsakis commented Jul 2, 2018

nikomatsakis commented Jul 2, 2018

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Jul 2, 2018

bors commented Jul 2, 2018

bors commented Jul 3, 2018

bors commented Jul 3, 2018

michaelwoerister commented Jul 5, 2018

nikomatsakis Jun 29, 2018 •

edited

Loading